fix(snapshots): reject mismatched resume Content-Range starts - #325

Reviewed at c93a4f3. I don't have a Rust toolchain in this environment, so this is a source review rather than a re-run of your test commands — I've flagged below where that limit matters. The fix is correct and does exactly what #319 asked for. One behavioural consequence is worth resolving before merge, and it isn't visible from the test as written.

What's right

• parse_content_range_start correctly extracts the start from bytes <start>-<end>/<total>.
• The guard is gated on is_partial && existing_size > 0 — the identical predicate that open_part_file(part_path, is_partial && existing_size > 0) uses to decide append-vs-truncate. Validating on exactly the condition that enables appending is the right invariant, and it means the 200-ignores-Range path and the existing_size == 0 path are provably untouched. No regression surface.
• Running the check before parse_total_size means a mismatched response is rejected before any file handle is opened.
• The CONTENT_RANGE constant swap is a real if small improvement.

Blocking: rejecting without clearing the .part turns self-healing corruption into a permanent wedge

On mismatch the function returns Err and leaves the 7-byte .part and its marker on disk. Following that through the call chain:

1. resumable_download's retry loop re-reads existing_size from that same file at the top of every iteration, so all ten attempts (MAX_DOWNLOAD_RETRIES = 10, RETRY_BACKOFF_SECS = 5) send the identical Range: bytes=7- to the same misbehaving cache. That's ~45s of backoff spent re-asking a question already answered.
2. When the loop gives up, nothing cleans up. force_download_and_extract_both only calls remove_dir_all(pair.tmp_dir) on extraction failure. download_archive's doc comment makes this explicit and deliberate: a failed download must leave the .part behind so a later run resumes "instead of transferring tens of gigabytes again."
3. On the next invocation, prepare_partial_download finds a matching url_identity marker and returns early — keeping the .part. Resume from 7 bytes, same rejection.

So the operator is stuck permanently until someone manually deletes the .part, and the error message doesn't hint that this is the remedy.

Compare the pre-fix behaviour I traced on #319: the corrupt append produced a bad archive, extraction failed, remove_dir_all(tmp_dir) fired, and the next run started clean. Silent corruption — but self-healing.

This PR removes a correctness bug and installs an availability bug in its place. That's a net improvement (a wedged download beats a corrupted datadir), but it's avoidable. #319 offered two options and this takes option 1; option 2 costs one line and has neither failure mode:

if range_start != existing_size {
    let _ = std::fs::remove_file(part_path);
    return Err(eyre::eyre!(
        "Server returned Content-Range starting at {range_start}, expected {existing_size}; discarding partial download"
    ));
}

Because attempt_download re-reads existing_size from disk on entry, attempt 2 then sees 0, sends no Range header, receives a plain 200, and open_part_file(.., false) truncates. The download recovers inside the same run, with no operator intervention. Keeping the marker is correct here — the URL hasn't changed, and the part is simply rebuilt from zero.

The test can't see any of this

It calls attempt_download directly, unlike all eleven sibling resumable_download_* tests which go through run_resumable_download. Three consequences:

• .expect(1) only holds because the retry loop is bypassed. Through the real entry point the mock would receive ten requests — itself worth asserting.
• It cannot observe the .part lifecycle across attempts, which is precisely what this change affects.
• Snapshot resume should reject mismatched Content-Range starts before appending to .part files #319's own suggestion was to "assert that run_resumable_download(...) returns an error," so this is narrower than the issue proposed.

The single-attempt test is a good unit test of the parse and should stay. Worth adding one alongside it that drives run_resumable_download and asserts the end state of the .part — that test is what would distinguish option 1 from option 2 above.

Correcting the note about #139

The description says #139 touches the same file but handles a different edge case. Semantically true, but I tested the merges rather than assuming, and the mechanics matter for whoever lands these:

#139 already conflicts with main on its own — its merge base is a85368c0, well behind — so it needs a rebase regardless and nothing here is #325's fault. Worth stating plainly so this PR doesn't get blamed for it.

That said, the two do collide textually. The import change auto-resolves because both PRs make a byte-identical edit, but both then rewrite the body of parse_total_size in different ways, and that conflicts:

<<<<<<< HEAD          (#325: inlined, using CONTENT_RANGE)
=======
        parse_content_range_total(response.headers())   (#139: extracted helper)
>>>>>>> pr139

If both land you end up with parse_content_range_total(&HeaderMap) and parse_content_range_start(&Response) side by side — two near-duplicate parsers of the same header with different signatures. A single parse_content_range() -> Option<ContentRange { start, end, total }> would serve both and remove the conflict entirely. Worth a word with #139's author about who absorbs it.

Also worth noting the two are genuinely complementary: #139's 416 handling covers the case where the .part is already complete, which is a neighbouring branch of the same resume logic.

Carried over from #319 (not blocking, scope discipline here is good)

Listing only because they live in the lines this PR touches, so they're cheap to fold in if a maintainer wants them:

• Content-Range: bytes 7-10/* is legal (RFC 9110 §14.4, complete-length may be *). The new start check passes, then parse_total_size does "*".parse::<u64>() → None → "Server did not provide Content-Length or Content-Range header", which is false — it did.
• strip_prefix("bytes ") is exact on case and spacing; range units are case-insensitive tokens per RFC 9110.
• An inverted range like bytes 7-3/11 still validates, since only the start is compared.
• The final .part length is still never compared against total before rename.

Good, tightly-scoped fix — the remove_file line is the one thing I'd want before it merges.

During follow-up testing, I found a second, adjacent Content-Range integrity case that is not covered by the current start-offset validation.

A server can return:

text
Range request: bytes=7-
Content-Range: bytes 7-10/11
Response body: 2 bytes

Here the returned range start correctly matches the 7-byte .part file, so this PR’s new check accepts the response. However, the header claims four bytes (7..=10) while the response body contains only two.

I reproduced this on the current main with a focused regression test. io::copy treats the early EOF as a successful copy, and attempt_download returns:

text
Ok(11)

The resulting file contains only 9 bytes, despite the reported total being 11. Through resumable_download, that incomplete file can then be promoted from .part to the final snapshot filename.

The failing assertion was:

text
short 206 response must not be accepted: Ok(11)

This is distinct from #319:

• Snapshot resume should reject mismatched Content-Range starts before appending to .part files #319 and this PR validate that the returned range starts at the requested offset.
• This case has the correct start offset but an incomplete response body.
• PR fix(snapshots): handle completed part-file resumes #139 addresses completed .part files receiving HTTP 416, so it does not cover this case either.

A possible additional invariant would be to verify that:

1. the copied response length equals Content-Range end - start + 1, and
2. the resulting .part file size equals the reported total before promotion.

Would you prefer this integrity check to be included in this PR because it touches the same resume path, or should I keep #319 narrowly scoped and open a separate follow-up issue?

Addressed the blocking resume-lifecycle issue in commit 0493c58.

Changes:

• A mismatched Content-Range response now removes the stale .part file before returning an error.
• The next retry therefore sees an existing size of zero and sends no Range header.
• The URL identity marker is retained during recovery and removed after the successful promotion.
• The existing single-attempt regression test now verifies that the stale .part file is discarded.
• A new resumable_download regression test verifies the complete recovery path:
  1. the first request resumes with Range: bytes=7-;
  2. the server returns a mismatched 206 response;
  3. the second request is sent without a Range header;
  4. the full response is downloaded and promoted successfully;
  5. the .part file and marker are removed.

Before the fix, both regression tests failed, and the end-to-end test retried the same range ten times before returning an error.

Verification after the fix:

text
Focused regression tests: 2 passed
Full arc-snapshots suite: 109 passed
cargo fmt: passed
cargo clippy --all-targets -D warnings: passed
git diff --check: passed

The separate short-body 206 case has not been included in this commit and remains pending the scope decision above.

@osr21, could you please take another look?

Re-reviewed at 0493c585. Both of your comments below. Still no Rust toolchain here, so this remains source review — I traced the call chain rather than re-running your suite.

1. The blocking issue is resolved, and you improved on what I suggested

The recovery path checks out end to end:

• The remove_file sits before open_part_file, so nothing holds a handle on the file being unlinked. That ordering also keeps it clean on Windows, where unlinking an open file behaves differently.
• Attempt 2 re-derives state from disk — std::fs::metadata(part_path).map(|m| m.len()).unwrap_or(0) returns 0 once the file is gone, so existing_size == 0, no Range header is sent, is_partial is false, and open_part_file(.., false) truncates. The recovery is a consequence of existing behaviour rather than new bookkeeping, which is what makes it robust.

On the one place you deviated from my suggestion — I proposed let _ = std::fs::remove_file(part_path); and you used ? with a distinct message. Yours is better. I traced both: if the unlink genuinely fails, both variants wedge identically, because the retry loop re-reads the same 7-byte .part, sends the same Range: bytes=7-, and hits the same rejection. The difference is only in what the operator sees, and Failed to discard mismatched partial download <path>: <error> names the actual cause, where my version would have kept reporting the Content-Range mismatch and hidden the real problem. Swallowing that error bought nothing.

The new resumable_download_recovers_from_mismatched_content_range_start test also closes the gap I raised about the original test bypassing the retry loop. Asserting the two-request sequence, Range present on the first and absent on the second, plus !part_path.exists() and !marker_path.exists(), pins the lifecycle rather than just the return value. That's the right shape.

Two cosmetic notes, neither worth a commit on their own:

• The unconditional RETRY_BACKOFF_SECS sleep still fires between attempts 1 and 2, so a self-corrected mismatch costs a pointless 5-second wait before the retry that was always going to succeed. Special-casing it probably isn't worth the branch, but it's there.
• The retry log then prints Retry attempt 2/10 - resuming from 0 bytes, which reads oddly for what is actually a restart.

Your commit message says the marker is "retained during recovery." True by construction — nothing on the mismatch path touches it — though the test only asserts it's absent at the end, not that it survived attempt 1. Since prepare_partial_download runs once before the loop, that wouldn't affect a single run either way, so I'd leave it.

From my side the blocking concern is cleared.

2. The short-body case — confirmed, and worse-looking than you described

I verified this independently and it holds. It isn't quite that io::copy "treats early EOF as success" — the byte count is computed and then explicitly thrown away:

let result = io::copy(&mut response, &mut writer).and_then(|_| writer.inner.flush());
println!();
result?;

Ok(total)

The |_| discards precisely the number needed to detect this, and total comes from parse_total_size(&response) — a header value. So attempt_download reports what the server claimed and never what it wrote. resumable_download then treats Ok as success and renames straight to the final path. There's no size comparison anywhere before that rename.

For the record, this is pre-existing: neither the io::copy line nor Ok(total) is touched by this PR, so it's present on main today and is not a regression from #319.

The fix is smaller than it looks, because the resume path already handles it

Worth knowing before anyone scopes it: your invariant (1) is self-sufficient. If a short body returned Err instead of Ok, the 9-byte .part stays on disk, the retry loop re-reads existing_size == 9, sends Range: bytes=9-, and the server answers Content-Range: bytes 9-10/11 — which passes the start check this PR just added, appends the missing 2 bytes, and completes at 11. It self-heals within the same run with no new recovery logic. Detection is the whole job; invariant (2) is then a cheap backstop rather than the mechanism.

Two things to write defensively when you get to it, both from the carried-over list in my first review:

• Content-Range: bytes 7-10/* is legal (RFC 9110 §14.4). end - start + 1 is still computable there, but parse_total_size currently chokes on *, so invariant (2) needs a different source of truth than invariant (1) in that case.
• An inverted range like bytes 7-3/11 currently validates, since only the start is compared — so end - start + 1 must not be allowed to underflow.

On severity, honestly

I'd calibrate this slightly below "silent corruption." A truncated .lz4/tar archive will almost certainly fail extraction, at which point force_download_and_extract_both fires remove_dir_all(pair.tmp_dir) and the next run starts clean — the same self-healing route the pre-fix #319 behaviour took. So the realistic blast radius is a wasted full download cycle and an error that surfaces a long way from its cause, rather than a corrupted datadir. Still worth fixing: the returned total is a false statement to every caller, and "extraction happens to fail" is a weak guarantee to rely on.

Scope: separate issue, please

To answer your question directly — I'd keep this PR as is and open a new issue.

1. It's pre-existing on main and has a different root cause. Snapshot resume should reject mismatched Content-Range starts before appending to .part files #319 is about offset alignment; this is about body completeness. Landing both here would leave a PR whose title describes half its content.
2. This PR has already grown once with the recovery work and is waiting on maintainer review. A second behavioural change resets that review.
3. It's arguably more severe than Snapshot resume should reject mismatched Content-Range starts before appending to .part files #319, and folding it into a follow-up commit here under-signals it. Its own issue lets maintainers triage it on its own merits — which is exactly the path Snapshot resume should reject mismatched Content-Range starts before appending to .part files #319 took.
4. fix(snapshots): handle completed part-file resumes #139 (open since July, same file, also rewriting this resume logic) already creates conflict surface in attempt_download. A third concurrent change to the same function compounds it.

When you file it, I'd include the self-healing observation above — it turns what reads like a significant integrity fix into a small one, which should help it get picked up.